-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Polylines on 3D Tiles #7437
Polylines on 3D Tiles #7437
Conversation
Thanks for the pull request @lilleyse!
Reviewers, don't forget to make sure that:
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple notes on specs, but otherwise nothing pops out to me.
(btw this is awesome)
var visualizer = new PolylineVisualizer(scene, objects); | ||
|
||
var polyline = new PolylineGraphics(); | ||
polyline.positions = new ConstantProperty([Cartesian3.fromDegrees(0.0, 0.0), Cartesian3.fromDegrees(0.0, 1.0)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @OmarShehata discovered a little while back, the sizes of geometries can actually impact spec runtime pretty severely. Do the ClassificationType
specs significantly increase runtime?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw an improvement when changing 1.0 to 0.000001. Master is ~3.5 seconds, this branch is ~5.0, with that change I see ~4.5. It varies a lot though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't confirmed this but I have reason to believe that a spec that takes ~5 seconds on our machines will time out from time to time (like here #7249). If there's no easy way to bring this down I'd mention the spec in that issue when that gets merged so I can take a look next time I'm cutting down test times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry for not stating that my times were for the whole file not a single test.
scene.initializeFrame(); | ||
var isUpdated = visualizer.update(time); | ||
scene.render(time); | ||
return isUpdated; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good way to wrap these pollToPromise
blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I cleaned it up in PolylineVisualizerSpec
and GeometryVisualizerSpec
.
🎉 In addition to the CZML example, please add/update a Sandcastle example using the CesiumJS API. |
This might be an issue that we don't fix. It'll possibly also be an issue with #7434. |
7fec168
to
56b63d1
Compare
5d5188b
to
3729e70
Compare
All the comments should be addressed now. I opened an issue for the slicing through problem: #7453. It will require a bit of thinking to get right so I think it should get pushed to some other PR. |
Follows up on #7434 and adds support for polylines on 3D Tiles.
Demo I've been using for testing: localhost link